fix: stop an unmatched repository inheriting a global identity by simonostendorf · Pull Request #12 · ostendorf-cloud/gitcd

simonostendorf · 2026-11-13T21:24:14Z

What

internal/gitconfig writes user.useConfigOnly unconditionally at the top of the managed
include file, not only inside the profile files.
internal/git finds identity keys the global config sets directly, wherever they sit, and can
remove them.
internal/config can add keys to a profile through yaml.Node, so a hand written
configuration keeps its comments.
cmd/gitcd init reports those keys, says why they defeat the per remote identity, and moves
them into a dedicated profile only when asked.

Why

Closes fix: useConfigOnly does not protect an unmatched repository when a global identity exists #8.
The gap had two halves, and the issue only described one. Removing the global keys is not
enough on its own: useConfigOnly was written only into the profile files, which are reached
through an includeIf, so it never applied to a repository no rule matches, which is the only
repository it was meant to protect. Both halves are needed for the guarantee to hold, and the
second was found by probing git rather than from the issue text.
The target is a dedicated profile, never an existing one chosen by guesswork. Merging into
whichever profile happens to exist would silently overwrite a key someone set on purpose, and
writing into defaultProfile would defeat the protection being added, since an unmatched
repository is supposed to receive nothing.

Verification

task check exit 0 after the rebase onto main.
End to end against an isolated HOME with a leaked global identity and a hand written
~/.gitcd.yaml:
- gitcd init reports both keys, names the profile they belong in, and moves nothing. The
  global config still carries them.
- gitcd init --move-identity=yes moves them into profile global, removes them from the
  global config, backs up both files, and says plainly that the new profile is not applied
  automatically.
- A repository matching no rule then fails to commit:
  fatal: no email was given and auto-detection is disabled. Before this change the same
  commit succeeded under the leaked identity.
- A matched repository still resolves its own profile.
The default never moves anything and never reads stdin, so a script or a CI run cannot be
made to move keys by accident.

Risk

--move-identity=ask prompts on stderr and reads one line from stdin. There is no terminal
detection, because that needs a dependency this repository does not carry, so interactivity is
opt in rather than detected. A closed or empty stdin resolves to no.
Editing ~/.gitcd.yaml preserves comments but reflows the file: indentation is normalised and
blank lines between top level sections are dropped. The content is correct and the comments
survive, but a hand written file does not come back byte identical.
init prints its report on stdout. Whether that counts as the command's result, which
AGENTS.md reserves stdout for, is worth a second opinion.

Closes #8

^{Open in [code]smith · Need help on this PR? Tag @codesmith-bot with what you need.}

simonostendorf · 2026-11-13T21:34:46Z

Verdict: changes requested

Reviewed at ebedba9, rebased onto main. Every probe below ran against a throwaway HOME under
$TMPDIR with GIT_CONFIG_NOSYSTEM=1. The real ~/.gitconfig is unchanged: 537 bytes, mtime
Oct 17 08:38:06 2026, md5 9b211170f1edca5a2adcd20cc7aa571b, no gitcd string, no backup file
written beside it.

The core of this change is right and I verified both halves independently.

With the keys moved out, a repository matching no rule and no defaultProfile refuses the commit:

--- effective user.email: ---
(unset)
--- effective useConfigOnly: ---
true
fatal: no email was given and auto-detection is disabled
commit exit=128

A matched repository still resolves:

file:.../profiles/work.gitconfig	work@example.test
[main (root-commit) a75919d] test: probe matched identity

The second half holds up. Removing just the unconditional block from renderIncludes and rerunning
the same scenario, with the global keys already moved out, the unmatched repository committed under
a guessed identity:

--- effective useConfigOnly in the unmatched repo: ---
(UNSET - the block is gone)
author=Simon Ostendorf <simonostendorf@Simons-MacBook-Pro.local>

So the unconditional block is load bearing, exactly as the commit message claims.

The destructive path is also well guarded. Ten attempts to make init move keys unasked all failed
to move anything: no flag, --move-identity=maybe, --move-identity=YES, --move-identity=,
--move-identity=no, --dry-run --move-identity=yes, and --move-identity=ask with closed stdin,
empty stdin, no and maybe later. None hung, none wrote. Both files are backed up before the
write, both backups are readable and hold the pre-move content, and the closing message names them.

task check exits 0 here, full suite including -race.

What stops this from being an approve is the profile the keys land in, and one data loss case.

Blocker

cmd/gitcd/init.go:76-78 and internal/config/edit.go:26 — the dedicated profile guarantee the
pull request body states ("never writes into an existing profile or into defaultProfile") is not
enforced anywhere in the code. Two concrete failures, both silent:

With no flag at all, against a config that already carries a deliberate profile named global:
```
profiles:
    global:
        user.email: deliberate@example.test   ->  leaked@example.test
        user.name: Deliberate Name            ->  Test User
```
With --profile=everyday where defaultProfile: everyday:
```
defaultProfile: everyday
profiles:
    everyday:
        user.email: everyday@example.test     ->  leaked@example.test
```
The second re-arms the exact bug this pull request fixes: the leaked identity is now the default
profile, so every unmatched repository inherits it again. Neither case warns.

Fix: in runInit, after resolving target, load the configuration and refuse with exit 1 when
target equals cfg.DefaultProfile, and refuse when cfg.Profiles[target] already exists unless
the user passes an explicit override flag. defaultMovedProfileName being a constant is not a
collision check.
internal/config/edit.go:64 — yaml.Unmarshal into a single yaml.Node decodes only the first
document, and yaml.Marshal at line 43 writes only that one back, so every document after the
first is silently dropped from the file this command rewrites. Probed with a two document
~/.gitcd.yaml; the second document was gone from the live file afterwards, recoverable only
from the backup, with nothing printed to say so.

Fix: decode with yaml.NewDecoder in a loop, and return an error naming the file when it holds
more than one document, rather than editing it. Refusing is fine here; silently truncating is not.

Should

cmd/gitcd/init.go:253-290 — the move succeeds and exits 0 even when the configuration it wrote
into cannot be loaded. Against a ~/.gitcd.yaml carrying a pre-existing unknown key,
init --move-identity=yes exited 0, removed user.name and user.email from the global git
config, and left a file that gitcd apply then rejects with config: unknown key: ... field totallyUnknownKey not found. The user is left with no identity in the global config and a gitcd
config that does not load. The comment above moveGlobalIdentity already argues for persisting
before destroying; persisting something unreadable does not satisfy that argument.

Fix: run config.LoadFrom against the rendered bytes before writing them, and abort before
backupGlobalConfig and the unset loop when it fails. Unknown key rejection itself is correct and
still works, which I checked separately.
internal/git/config.go:271-345 — FindGlobalIdentityValues reimplements git config parsing from
raw text, and parseConfigValue documents that it will not strip an inline comment. A global
email = me@example.test # work therefore moves the literal value me@example.test # work into
the profile and then unsets the real key. Consider git config --global --no-includes --get-all
for the values and keep the text scan only for deciding which keys are present, so git's own
parser owns quoting and comments. If the text scan stays, say in the doc comment what happens to
a value it cannot parse faithfully.

Nit

cmd/gitcd/init.go:128 — decideMove runs before the *dryRun branch at line 132, so
--dry-run --move-identity=ask prompts on stderr and consumes a line of stdin for an answer it
then ignores. Move the *dryRun check above the prompt.
cmd/gitcd/init.go:284 — the configuration file is created 0o644 while both backups are
0o600. It carries the identity just removed from a 0o600 file. Use 0o600 for consistency.
internal/git/config_test.go:391 — the case named "an includeIf section is scanned too, not only
a bare include" asserts want: nil. The name states the opposite of the behaviour it pins. Rename
to say that a key under an includeIf header belongs to section includeif and is therefore not
an identity key.
cmd/gitcd/init.go:203 — reportGlobalIdentityKeys prints to stdout, which AGENTS.md reserves
for a command's result. init already printed status to stdout before this change, so this is a
pre-existing convention question rather than a regression, and the pull request is right to raise
it. Worth its own issue covering every init line rather than settling it here.
cmd/gitcd/init.go:206 — the report echoes user.signingkey's value to stdout. No value reaches
a log line or any error gitcd constructs, which I checked under --verbose, and
ConfigUnsetGlobal never receives a value, so AGENTS.md's secrets rule is met. Printing the key
name and the file, with the value only under --verbose, would still be the safer default for a
stream that gets piped and pasted.

Scope

--profile is surface issue 8 did not ask for and is the source of half the first blocker. Dropping
it, or gating it as above, would shrink the risk. The unconditional useConfigOnly block is beyond
the issue text but is necessary for the stated goal and is proven so above; it belongs here.

Tests have real teeth. I reverted the code behind three of them in a detached scratch copy and each
failed: TestRender_UnconditionalUseConfigOnly and
TestRunInit_UnmatchedRepositoryRefusesToCommitAfterMove both fail with the unconditional block
removed, and TestRunInit_ReportsGlobalIdentityKeysWithoutMovingByDefault fails when decideMove
defaults to moving.

Next round through the implement skill.

simonostendorf · 2026-11-13T22:15:33Z

Verdict: changes requested

Round two, at d6d489b rebased onto main. Every probe ran against a throwaway HOME under
$TMPDIR with GIT_CONFIG_GLOBAL and GIT_CONFIG_NOSYSTEM=1. The real ~/.gitconfig is
unchanged: 537 bytes, mtime Oct 17 08:38:06 2026, md5 9b211170f1edca5a2adcd20cc7aa571b, no
gitcd string, no gitcd file beside it.

task check exits 0 here, the full suite including -race.

Round one's findings

Blocker 1, the moved keys landing in defaultProfile or an existing profile: fixed.
--move-identity=yes --profile=everyday against defaultProfile: everyday exits 1 with
profile "everyday" is the defaultProfile in ..., and both files keep their md5. The same run
with no flag against a config already carrying a profile named global exits 1 with
profile "global" already exists in .... Neither leaves a backup or a temp file behind, because
the check sits before the first write.
Blocker 2, a multi document file losing every document after the first: fixed. A two document
~/.gitcd.yaml exits 1 with config: the existing configuration holds more than one YAML document, both documents survive byte for byte, and the global identity is untouched. A file
ending in a bare --- is still accepted, which is the right line to draw.
Should, exiting 0 after removing the global keys when the written config does not load: fixed
for the case it was reported against. totallyUnknownKey: 1 now exits 1 with the updated configuration would not load, aborting before touching ..., the global keys stay, and no
.gitcd-validate-tmp is left behind. But the guarantee it was meant to establish still does not
hold in general, see the blocker below.

Tests have teeth. Each fix reverted on its own in a detached, source only scratch copy fails its
test: TestRunInit_RefusesToTargetADeliberateProfileThatIsAlsoTheDefault and
TestRunInit_RefusesToOverwriteAPreExistingProfileNamedGlobal both fail with
targetProfileConflict's two refusals disabled, the multi document subtest fails with the
nextDocumentExists call removed, and TestRunInit_AbortsTheMoveWhenTheUpdatedConfigWouldNotLoad
fails with the temp file and LoadFrom replaced by the old direct write.

The error prefix cleanup is clean: all eleven sites keep their own context, the caller supplies
gitcd init:, and the two plain returns carry errors that already name config:. No information
lost, with one exception noted as a nit below.

Blocker

internal/config/edit.go:39-43 — a profiles: key with an empty value makes init report a
move that did not happen and then remove the identity from the global git config anyway. This is
the outcome the whole change exists to prevent, reached with a file a person plausibly writes by
hand.

~/.gitcd.yaml:
```
root: ~/Code
profiles:
```
gitcd init --move-identity=yes:
```
backed up .../.gitcd.yaml to .../.gitcd.yaml.gitcd-backup-20261113T221101Z
moved user.name, user.email into profile "global" in .../.gitcd.yaml
removed user.name, user.email from .../.gitconfig
exit=0
```
~/.gitcd.yaml afterwards, byte for byte identical to before:
```
root: ~/Code
profiles:
```
git config --global --get user.email then exits 1. The identity exists nowhere, only in the
backup, and nothing printed says so.

Cause: mappingChild returns any existing value node without checking its kind. profiles:
with no value is a !!null scalar node, so the key and value nodes are appended to a scalar's
Content, which yaml.Marshal ignores. The round one Should does not catch it, because the
unchanged file loads perfectly well. defaultProfile: null behaves the same way.

Fix: in mappingChild, treat an existing child that is not a MappingNode as something to
replace when it is an empty !!null scalar, and to refuse otherwise, and have AddProfileKeys
return an error rather than a document. A cheap belt and braces on top: after
config.LoadFrom(tmpPath) succeeds, assert that every moved key is actually present in
cfg.Profiles[target] before the rename, so no future shape of this class can report a move
that did not happen.

Should

cmd/gitcd/init.go:378-388 — the temp file plus rename replaces a symlinked ~/.gitcd.yaml
with a regular file. With the config symlinked into a dotfiles repository, the common setup, the
run exits 0, ~/.gitcd.yaml is a plain file afterwards, and the dotfiles copy still holds the
old content, so the moved identity is no longer tracked and the link is silently gone. The
previous direct os.WriteFile wrote through the link. Resolve the symlink with
filepath.EvalSymlinks before the rename, or write through the link once validation passed.
cmd/gitcd/init.go:378 — the validation file has a fixed, predictable name in the home
directory and is written with os.WriteFile, which follows a symlink. With a symlink already
sitting at ~/.gitcd.yaml.gitcd-validate-tmp, the probe wrote the rendered config through it
into the link's target and left ~/.gitcd.yaml as a symlink to that target. A stale leftover is
merely overwritten, which is fine, but os.CreateTemp in the same directory, or
O_CREATE|O_EXCL|O_NOFOLLOW, removes the class.
cmd/gitcd/init.go:280-285 — the lenient conflict parse returns "no conflict" on any parse
error, so a profiles or defaultProfile of the wrong YAML type skips both refusals. Nothing
is lost today, since config.LoadFrom rejects every such shape before the rename, which I
confirmed for a list, a scalar and an undefined defaultProfile: all three exit 1 with the
global keys intact. But the guard rests entirely on the stricter parse happening to be stricter
in exactly the same places, and profiles: null is already one shape where neither parse
objects. Distinguish "the file does not parse" from "the file parses and has no conflict", and
refuse on the former.

Nit

internal/config/edit.go:57 — ErrMultipleDocuments does not name the file, so the refusal
reads gitcd init: config: the existing configuration holds more than one YAML document with no
path, and the config may sit at either candidate path. Round one asked for the file name.
cmd/gitcd/init.go:384 — the abort message quotes the temp path
(.gitcd.yaml.gitcd-validate-tmp), a file that is already deleted by the time the user reads it.
Report cfgPath instead.
cmd/gitcd/init.go:348-354 — the config backup is written before AddProfileKeys and before
validation, so a refused run leaves a .gitcd-backup- copy of a file that never changed.
The round one nits still stand: decideMove prompting under --dry-run, the config written
0o644 while both backups are 0o600, the includeIf test name asserting the opposite of its
want: nil, and the signing key value going to stdout. On the last one I confirmed again that no
value reaches a log line or any error gitcd constructs: only reportGlobalIdentityKeys prints
values, ConfigUnsetGlobal receives a key name alone, and --verbose adds only paths.

Rounds are exhausted on this pull request, so the blocker needs a human to pick up rather than
another automated round.

The key was written only into the profile files, which are reached through an includeIf, so it never applied to a repository no rule matches. That is the only repository it was meant to protect. It is written unconditionally at the top of the managed include file now. Refs #8

Position agnostic, unlike the existing ordering check: a key set directly in the global config wins for every unmatched repository wherever it sits. Refs #8

The edit goes through yaml.Node rather than a struct round trip, so a hand written configuration keeps the comments its author put there. Refs #8

init names every identity key the global config sets directly, says why it defeats the per remote identity, and moves them into a dedicated profile only when asked. The default never moves and never reads stdin, and both files are backed up before a write. Refs #8

Refs #8

Nothing enforced the promise that the keys never land in an existing profile or in the defaultProfile. Targeting the default made every unmatched repository inherit the leaked identity again, the exact bug the move exists to fix, and a deliberate value was overwritten in silence either way. The updated configuration is also written and validated before anything is removed from the global config, so a file that would not load no longer leaves the user with no identity at all, and a multi document file is refused by name rather than truncated. Refs #8

A bare "profiles:" key decodes to a null scalar, and appending to a scalar is dropped on marshal, so init reported success, removed the global identity and wrote nothing. The identity survived only in the backup. A null child becomes a real mapping now, and any other shape is refused rather than coerced. Beyond the cause, the rendered bytes are parsed back and checked to carry the keys before anything is removed, so a write that does not contain what it claimed can no longer be followed by a removal. A symlinked configuration is resolved and written through, keeping a dotfiles checkout intact, and the validation file no longer has a predictable name. Refs #8

simonostendorf · 2026-11-13T23:03:25Z

Verdict: approve

Round three, scoped to round two's finding list and what the rework introduced. task check is green at the pushed head rebased onto main (exit 0, every lint target plus go test -race). Every probe below ran against a throwaway HOME with GIT_CONFIG_NOSYSTEM=1.

Round two's findings

Blocker, a bare profiles: decoding to a null scalar: fixed. profiles: and a bare profile name now become real mappings, the keys land, and the removal only follows a verified write. Reverting the null-to-mapping branch alone fails TestAddProfileKeys/writes_into_a_bare_profiles_key_... and .../writes_into_a_profile_name_already_present_...; reverting it together with the landed-keys call reproduces the original destruction and fails TestRunInit_NeverClaimsSuccessWhileLosingAMoveIntoABareProfilesKey.
Should 1, the lenient conflict parse waving a move through: fixed. profiles as a list, as none, as "work", and a file with two profiles: keys are all refused with the "does not parse well enough" message, exit 1, with neither file changed.
Should 2, a symlinked ~/.gitcd.yaml replaced by a regular file: fixed. Also holds when the parent directory is the symlink: ~/.config -> .../real, the config at ~/.config/gitcd/config.yaml, the link intact afterwards and real/gitcd/config.yaml carrying the move.
Should 3, the predictable validation temp path: fixed. os.CreateTemp gives it a random name, the deferred remove clears it on every path checked, and a failed rename into a read-only target directory left nothing behind.
Nit, ErrMultipleDocuments not naming the file: fixed. The single call site wraps every AddProfileKeys error with cfgPath.

New findings

Should

The landed-keys guard is not grafted at its call site. Deleting the identityKeysLanded(cfg, target, found) call from moveGlobalIdentity keeps the whole suite green (go test ./cmd/... ./internal/config/... both ok). TestIdentityKeysLanded only exercises the helper in isolation, and the end-to-end test only fails once the mappingChild fix is gone too. The second guard can be deleted by a later refactor with nothing noticing.
An existing ~/.gitcd.yaml at mode 0600 comes back 0644. The rename carries the temp file's own mode, which is forced to 0644, over whatever the file had. Stat the existing file and chmod the temp copy to its mode, falling back to 0644 when there is no file yet. This predates the rework, it is in the round two write path too, and the file holds no secret, so it is not a blocker.

Nit

One message mixes path forms. moved ... in /private/tmp/... next to backed up /tmp/... in the same output, and in the symlinked-parent case the backup line names ~/.config/gitcd/config.yaml while the moved line names .../real/gitcd/config.yaml. Both are correct, and naming the real dotfiles file is useful, but print cfgPath and add the resolved path only when it differs, rather than silently swapping one for the other.
A validation failure names a file the user cannot look at: the updated configuration would not load ...: config: parse /Users/x/.gitcd.yaml.gitcd-validate-tmp-138583564: config: unknown key: field mirror not found. The temp file is already removed by then. Name cfgPath in that message instead.
A broken symlink at ~/.gitcd.yaml, a dotfiles checkout not yet present, is replaced by a regular file: EvalSymlinks reports ErrNotExist and the write falls back to cfgPath. Nothing is lost and no alternative is obviously better, but the doc comment claims the link is kept, which is not true for this shape.
A refusal that happens after the configuration backup, an aliased or anchored profiles for instance, leaves the .gitcd-backup- file behind even though nothing changed.

Nothing in the diff outside round two's list looks wrong: the ConfigUnsetGlobal exit code 5 handling, the configKeyLine value capture and parseConfigValue, the unconditional [user] useConfigOnly block and its golden file, and the concept doc all read as intended.

simonostendorf added bug Something isn't working priority: high High priority stack: go Related to Go code labels Nov 13, 2026

simonostendorf force-pushed the fix/8-global-identity branch from ebedba9 to d6d489b Compare November 13, 2026 22:06

simonostendorf added 7 commits November 13, 2026 23:52

feat(git): find and remove a global identity key

db91ef8

Position agnostic, unlike the existing ordering check: a key set directly in the global config wins for every unmatched repository wherever it sits. Refs #8

feat(config): add keys to a profile without losing comments

a12b789

The edit goes through yaml.Node rather than a struct round trip, so a hand written configuration keeps the comments its author put there. Refs #8

docs: state what useConfigOnly does and does not protect

2fbe1ce

Refs #8

simonostendorf force-pushed the fix/8-global-identity branch from d6d489b to ce2b3f4 Compare November 13, 2026 22:52

simonostendorf marked this pull request as ready for review November 13, 2026 23:04

simonostendorf merged commit fb5bc38 into main Nov 13, 2026
10 checks passed

simonostendorf deleted the fix/8-global-identity branch November 13, 2026 23:09

ostendorf-cloud-release Bot mentioned this pull request Nov 13, 2026

chore(main): release v0.1.0 #9

Open

Navigation Menu

Repository navigation

fix: stop an unmatched repository inheriting a global identity - #12

fix: stop an unmatched repository inheriting a global identity#12
simonostendorf merged 7 commits into
mainfrom
fix/8-global-identity

simonostendorf commented Nov 13, 2026 •

edited by blacksmith-sh Bot

Loading

Uh oh!

simonostendorf commented Nov 13, 2026

Uh oh!

simonostendorf commented Nov 13, 2026

Uh oh!

simonostendorf commented Nov 13, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

Conversation

simonostendorf commented Nov 13, 2026 • edited by blacksmith-sh Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Verification

Risk

Uh oh!

simonostendorf commented Nov 13, 2026

Blocker

Should

Nit

Scope

Uh oh!

simonostendorf commented Nov 13, 2026

Round one's findings

Blocker

Should

Nit

Uh oh!

simonostendorf commented Nov 13, 2026

Round two's findings

New findings

Should

Nit

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

simonostendorf commented Nov 13, 2026 •

edited by blacksmith-sh Bot

Loading